Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[query] no unnecessary object allocations in RegionMemory.allocate #13794

Merged
merged 2 commits into from
Oct 17, 2023

Conversation

danking
Copy link
Contributor

@danking danking commented Oct 11, 2023

Consider this:

class Foo {
   def bar(): (Long, Long) = (3, 4)

   def destructure(): Unit = {
     val (x, y) = bar()
   }

   def accessors(): Unit = {
     val zz = bar()
     val x = zz._1
     val y = zz._2
   }
}

image

These should be exactly equivalent, right? There's no way Scala would compile the match into something horrible. Right? Right?

public void destructure();
  Code:
     0: aload_0
     1: invokevirtual #27                 // Method bar:()Lscala/Tuple2;
     4: astore_3
     5: aload_3
     6: ifnull        35
     9: aload_3
    10: invokevirtual #33                 // Method scala/Tuple2._1$mcJ$sp:()J
    13: lstore        4
    15: aload_3
    16: invokevirtual #36                 // Method scala/Tuple2._2$mcJ$sp:()J
    19: lstore        6
    21: new           #13                 // class scala/Tuple2$mcJJ$sp
    24: dup
    25: lload         4
    27: lload         6
    29: invokespecial #21                 // Method scala/Tuple2$mcJJ$sp."<init>":(JJ)V
    32: goto          47
    35: goto          38
    38: new           #38                 // class scala/MatchError
    41: dup
    42: aload_3
    43: invokespecial #41                 // Method scala/MatchError."<init>":(Ljava/lang/Object;)V
    46: athrow
    47: astore_2
    48: aload_2
    49: invokevirtual #33                 // Method scala/Tuple2._1$mcJ$sp:()J
    52: lstore        8
    54: aload_2
    55: invokevirtual #36                 // Method scala/Tuple2._2$mcJ$sp:()J
    58: lstore        10
    60: return

public void accessors();
  Code:
     0: aload_0
     1: invokevirtual #27                 // Method bar:()Lscala/Tuple2;
     4: astore_1
     5: aload_1
     6: invokevirtual #33                 // Method scala/Tuple2._1$mcJ$sp:()J
     9: lstore_2
    10: aload_1
    11: invokevirtual #36                 // Method scala/Tuple2._2$mcJ$sp:()J
    14: lstore        4
    16: return

Yeah, so, it extracts the first and second elements of the primitive-specialized tuple, constructs a (java.lang.Long, java.lang.Long) Tuple constructs another primitive-specialized tuple (for no reason???), then does the match on that.

sigh.

Consider this:

```scala
class Foo {
   def bar(): (Long, Long) = (3, 4)

   def destructure(): Unit = {
     val (x, y) = bar()
   }

   def accessors(): Unit = {
     val zz = bar()
     val x = zz._1
     val y = zz._2
   }
}
```

These should be exactly equivalent, right? There's no way Scala would compile the match into
something horrible. Right? Right?

```
public void destructure();
  Code:
     0: aload_0
     1: invokevirtual hail-is#27                 // Method bar:()Lscala/Tuple2;
     4: astore_3
     5: aload_3
     6: ifnull        35
     9: aload_3
    10: invokevirtual hail-is#33                 // Method scala/Tuple2._1$mcJ$sp:()J
    13: lstore        4
    15: aload_3
    16: invokevirtual hail-is#36                 // Method scala/Tuple2._2$mcJ$sp:()J
    19: lstore        6
    21: new           #13                 // class scala/Tuple2$mcJJ$sp
    24: dup
    25: lload         4
    27: lload         6
    29: invokespecial hail-is#21                 // Method scala/Tuple2$mcJJ$sp."<init>":(JJ)V
    32: goto          47
    35: goto          38
    38: new           hail-is#38                 // class scala/MatchError
    41: dup
    42: aload_3
    43: invokespecial hail-is#41                 // Method scala/MatchError."<init>":(Ljava/lang/Object;)V
    46: athrow
    47: astore_2
    48: aload_2
    49: invokevirtual hail-is#33                 // Method scala/Tuple2._1$mcJ$sp:()J
    52: lstore        8
    54: aload_2
    55: invokevirtual hail-is#36                 // Method scala/Tuple2._2$mcJ$sp:()J
    58: lstore        10
    60: return

public void accessors();
  Code:
     0: aload_0
     1: invokevirtual hail-is#27                 // Method bar:()Lscala/Tuple2;
     4: astore_1
     5: aload_1
     6: invokevirtual hail-is#33                 // Method scala/Tuple2._1$mcJ$sp:()J
     9: lstore_2
    10: aload_1
    11: invokevirtual hail-is#36                 // Method scala/Tuple2._2$mcJ$sp:()J
    14: lstore        4
    16: return
```

Yeah, so, it extracts the first and second elements of the primitive-specialized tuple, constructs
a `(java.lang.Long, java.lang.Long)` Tuple, then does the match on that.

sigh.
Copy link
Collaborator

@patrick-schultz patrick-schultz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. Can you leave a comment? Otherwise someone is liable to go back to the "nice" version.

@danking danking merged commit 30ec661 into hail-is:main Oct 17, 2023
8 checks passed
@ehigham
Copy link
Member

ehigham commented Oct 20, 2023

Ugh. Can you leave a comment? Otherwise someone is liable to go back to the "nice" version.

I was just about to do this then I saw the comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants